ci(event-names): add cross-repo EVENT_NAMES sync gate#101
ci(event-names): add cross-repo EVENT_NAMES sync gate#101daniloleonecarneiro wants to merge 2 commits into
Conversation
Reviewer's GuideAdds a Bash-based CI gate to ensure the canonical EVENT_NAMES lists in the CRM Ruby code and evo-flow TypeScript stay synchronized, and wires it into the GitHub Actions workflow with a change-detection short-circuit and a hard-coded expected count to force coordinated updates. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
Detect relevant changesstep relies ongithub.event.pull_request.base.sha/head.sha, which will be empty on non-PR events; consider adding anif: github.event_name == 'pull_request'guard on the job or step to avoid failures if the workflow is ever reused for push triggers. - The hard-coded
EXPECTED_COUNT=16incheck-event-names-sync.shintroduces a third location to touch on every legitimate list change; you might consider deriving the expected count from one canonical side (e.g., Ruby) or from a dedicated config file to avoid having to remember to bump this constant manually.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `Detect relevant changes` step relies on `github.event.pull_request.base.sha`/`head.sha`, which will be empty on non-PR events; consider adding an `if: github.event_name == 'pull_request'` guard on the job or step to avoid failures if the workflow is ever reused for push triggers.
- The hard-coded `EXPECTED_COUNT=16` in `check-event-names-sync.sh` introduces a third location to touch on every legitimate list change; you might consider deriving the expected count from one canonical side (e.g., Ruby) or from a dedicated config file to avoid having to remember to bump this constant manually.
## Individual Comments
### Comment 1
<location path="scripts/check-event-names-sync.sh" line_range="58-59" />
<code_context>
+ if (match(line, /\]/)) { print substr(line, 1, RSTART - 1); capture = 0; exit }
+ print line
+ }
+ ' "$TS_FILE" | grep -oE "'[a-z][a-z._]*'" | tr -d "'" | sort -u
+)
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The TS event-name regex is very restrictive and will silently ignore many valid identifier shapes.
`grep -oE "'[a-z][a-z._]*'"` only matches lowercase letters plus `.` and `_`, so any event name with digits, hyphens, uppercase letters, etc. is excluded from `ts_list`. That can mask divergence between the TS file and the other list. Please broaden the regex (e.g. `"'[-A-Za-z0-9._]+'"`) or otherwise match the full set of valid event-name characters so the check fails whenever either file adds an out-of-pattern name.
```suggestion
' "$TS_FILE" | grep -oE "'[-A-Za-z0-9._]+'" | tr -d "'" | sort -u
)
```
</issue_to_address>
### Comment 2
<location path="scripts/check-event-names-sync.sh" line_range="37-46" />
<code_context>
+
+# Ruby side: scope to the %w[ ... ] block, then tokenise on whitespace.
+ruby_list=$(
+ awk '
+ /%w\[/ { capture = 1; sub(/.*%w\[/, "") }
+ capture {
+ line = $0
+ if (sub(/\].*/, "", line)) { print line; capture = 0; exit }
+ print line
+ }
+ ' "$RUBY_FILE" | tr -s '[:space:]' '\n' | sed '/^$/d' | sort -u
+)
+
+# TS side: scope to the EVENT_NAMES = [ ... ] block, THEN extract single-quoted
+# literals. Anchoring to the block prevents a stray single-quoted string in a
+# comment, JSDoc, or future export from leaking into the canonical set.
+ts_list=$(
+ awk '
+ /EVENT_NAMES[[:space:]]*=[[:space:]]*\[/ { capture = 1 }
+ capture {
+ line = $0
</code_context>
<issue_to_address>
**suggestion:** TS block detection for `EVENT_NAMES` does not handle type annotations or alternative spacing around the `=`.
The current awk guard `/EVENT_NAMES[[:space:]]*=[[:space:]]*\/` only matches when `EVENT_NAMES` is immediately followed by `=` and `[`. With a type annotation or inline comment (e.g. `export const EVENT_NAMES: readonly string[] = [`), the match fails and the script sees an empty list, causing confusing sync failures. Consider allowing arbitrary chars between the name and `=` (e.g. `/EVENT_NAMES[^=]*=[[:space:]]*\[/`) so annotations/comments don’t break extraction.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| ' "$TS_FILE" | grep -oE "'[a-z][a-z._]*'" | tr -d "'" | sort -u | ||
| ) |
There was a problem hiding this comment.
suggestion (bug_risk): The TS event-name regex is very restrictive and will silently ignore many valid identifier shapes.
grep -oE "'[a-z][a-z._]*'" only matches lowercase letters plus . and _, so any event name with digits, hyphens, uppercase letters, etc. is excluded from ts_list. That can mask divergence between the TS file and the other list. Please broaden the regex (e.g. "'[-A-Za-z0-9._]+'") or otherwise match the full set of valid event-name characters so the check fails whenever either file adds an out-of-pattern name.
| ' "$TS_FILE" | grep -oE "'[a-z][a-z._]*'" | tr -d "'" | sort -u | |
| ) | |
| ' "$TS_FILE" | grep -oE "'[-A-Za-z0-9._]+'" | tr -d "'" | sort -u | |
| ) |
| awk ' | ||
| /%w\[/ { capture = 1; sub(/.*%w\[/, "") } | ||
| capture { | ||
| line = $0 | ||
| if (sub(/\].*/, "", line)) { print line; capture = 0; exit } | ||
| print line | ||
| } | ||
| ' "$RUBY_FILE" | tr -s '[:space:]' '\n' | sed '/^$/d' | sort -u | ||
| ) | ||
|
|
There was a problem hiding this comment.
suggestion: TS block detection for EVENT_NAMES does not handle type annotations or alternative spacing around the =.
The current awk guard /EVENT_NAMES[[:space:]]*=[[:space:]]*\/ only matches when EVENT_NAMES is immediately followed by = and [. With a type annotation or inline comment (e.g. export const EVENT_NAMES: readonly string[] = [), the match fails and the script sees an empty list, causing confusing sync failures. Consider allowing arbitrary chars between the name and = (e.g. /EVENT_NAMES[^=]*=[[:space:]]*\[/) so annotations/comments don’t break extraction.
Adds scripts/check-event-names-sync.sh — pure Bash, no Ruby/Node runtime, that extracts the canonical event-names list from evo-ai-crm-community/lib/events/evo_flow_event_names.rb and the TS mirror in evo-flow/src/modules/events/event-names.enum.ts, sorts and diffs them. Both sides use scoped extraction (awk anchored to the %w[ ] block on Ruby and EVENT_NAMES = [ ] on TS) so a stray quoted string in a comment cannot leak in. EXPECTED_COUNT=16 catches the case where both sides grow to the same wrong size in lockstep — bump it in the same PR that adds entries.
daniloleonecarneiro
force-pushed
the
danilocarneiro/evo-1241-freeze-evoflow-event-names
branch
from
de23a0c to
3b51c87
Compare
daniloleonecarneiro
changed the title
|
Code review — EVO-1241 (cross-repo sync gate) Script logic is correct (AC5 ✅): Ruby 🔴 HIGH — the gate is skipped on exactly the PRs that change event names. The Detect relevant changes step filters with So on a pointer-bump PR (the real path event names travel through), CI context: the 3 red checks on this PR (sync, dockerfiles, compose) are not caused by this PR's code — they all die at 🟢 LOW — the CRM README (#90) does not mention the No verdict here — awaiting Davidson's decision (including the AC6 workflow question above). |
The previous "Detect relevant changes" gate filtered the diff by path prefix (`^evo-ai-crm-community/lib/events/`, `^evo-flow/src/modules/events/`), which is the exact pattern `git diff --name-only` does NOT emit for the workflow that matters most here: a submodule-pointer bump. Bumps appear as a bare directory line (`evo-ai-crm-community`, `evo-flow`), so the gate evaluated to `run=false` and the sync check was silently skipped precisely when one side moved to a new EVENT_NAMES list and the other did not — defeating AC6 of the story. Drop the gate entirely. The script is pure bash and finishes in ~5s, so the cost of running unconditionally is negligible compared to the cost of chasing regex coverage for every diff shape git can emit.
|
Code review — EVO-1241 — Round 2 (delta vs the 03:03 review) The high finding from round 1 is resolved:
Two residuals on AC6 — restating so "H-1 resolved" isn't mistaken for "AC6 fully enforced":
🟢 L-3 (EXPECTED_COUNT doc) was addressed on evo-ai-crm #90's README (+ evo-flow already had it). The monorepo README still doesn't restate the rule — minor, optional. No verdict from me — consolidated status is on the Linear card. |
dpaes
left a comment
dpaes
left a comment
There was a problem hiding this comment.
✅ Approved (round-2 code review).
H-1 is resolved — the path-filter gate is gone and event-names-sync now runs unconditionally (submodules: recursive + the bash script), so a submodule-bump PR (the case that was being skipped) is now covered. Script logic re-verified; lists 16 ↔ 16, EXPECTED_COUNT=16 matches.
Two residuals noted on the Linear card so "H-1 resolved" isn't read as "AC6 fully enforced":
⚠️ For the gate to actually block merge, branch protection ondevelopmust listEvoFlow event-names syncas a required check (repo setting, outside this diff).⚠️ This job only goes green after #102 merges (submodules: recursivetrips on the orphan.claudegitlink until then) and after the submodule pointers are bumped. Merge last in the chain.
|
Status update: #102, #90 and #3 are merged. This PR (#101) is approved but intentionally held open — the submodule-pointer bump was excluded from this pass, so the To finish the chain: bump the |
|
Closing this PR per Davidson's decision: the cross-repo CI sync gate is not needed right now. The CI job and the version bump will be addressed later in a follow-up. |
2 participants
EVO-1241 — Cross-repo EVENT_NAMES sync gate (monorepo)
Terceira e última PR coordenada da story 7.5. CRM agora raise em
event_namedesconhecido;evo-flowagora 400 emevent/eventNamedesconhecido. Esta PR adiciona o terceiro pilar: um job de CI que bloqueia o merge de qualquer PR que mexa em um lado da lista canônica sem mexer no outro.Sem este gate, alguém adicionando
'order.placed'só no CRM (ou só no TS) ia mergear, e o sistema voltava a ter silent data gap em produção até alguém perceber.Mudanças
scripts/check-event-names-sync.sh(novo,chmod +x)%w[ ... ]via awk scoped) e do TS (EVENT_NAMES = [ ... ]via awk scoped +grep -oE "'...'"),sort -uem ambos,diff -u. Exit 0 em match +EXPECTED_COUNT=16. Exit 1 com diff (DIVERGENT) caso contrário..github/workflows/ci.ymlevent-names-sync.submodules: recursive+fetch-depth: 0(precisa do diffbase...head). StepDetect relevant changesfaz short-circuit quando nenhum arquivo relevante mudou no PR — não custa nada em PRs de docker/frontend.Como o script lida com falsos positivos
Antes do review adversarial:
grep -oE "'[a-z][a-z._]*'"em todo o arquivo. Um comentário tipo// see legacy 'foo.bar'seria contado como evento canônico. Comentário no script afirmava "EVENT_NAMES is the only such block" — mas o regex não enforçava.Agora:
awkscoped no blocoEVENT_NAMES = […]antes do grep. Stray quoted strings em comentários ou outras consts são ignorados.'stray.name'em comentário → OK, não conta).EXPECTED_COUNT— por quêHard-coded em
16. Detecta o caso patológico em que alguém adiciona um entry em ambos os lados simultaneamente mas o restante do PR não toca a story 7.5 — i.e., crescer a lista sem coordenação com integration. Ao bumpar de 16 → 17, o autor é forçado a editar o script no mesmo commit, o que dispara revisão.Trade-off: quando a lista cresce legitimamente, é uma terceira edição obrigatória (Ruby + TS + script). Documentado em ambos os READMEs (CRM + evo-flow).
CI — short-circuit
Filtro em job-level (
paths:no workflow) não cabe porque o workflow guardavalidate-composeelint-dockerfilestambém — não dá pra restringir o trigger sem desligar os outros. Solução: short-circuit por step.Validação local
python3 -c 'import yaml; ...')validate-compose,lint-dockerfiles,event-names-syncPRs coordenadas (mesma branch)
Merge order: CRM → evo-flow → esta PR. O gate só roda meaningfully depois das duas primeiras mergeadas (precisa dos arquivos canônicos atualizados em
developde cada submodule).EvoFlow::InvalidEventName+ F4 exceptions@IsIn(EVENT_NAMES)nos DTOs + e2eOut of Scope
paths:no nível do workflow (interferiria comvalidate-compose/lint-dockerfiles). Short-circuit por step cobre o caso.EXPECTED_COUNTdegit log(overkill).Test plan
🤖 Generated with Claude Code
Summary by Sourcery
Add a CI gate to enforce synchronization between the canonical CRM Ruby event names list and the evo-flow TypeScript mirror.
New Features:
Enhancements:
CI: